-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
security audit fixes #23
Conversation
vpn/bytes.go
Outdated
@@ -113,6 +113,10 @@ func bytesUnpadPKCS7(b []byte, blockSize int) ([]byte, error) { | |||
|
|||
// bytesPadPKCS7 returns the PKCS#7 padding of a byte array. | |||
func bytesPadPKCS7(b []byte, blockSize int) ([]byte, error) { | |||
if blockSize == 0 { | |||
return nil, fmt.Errorf("%w: %s", errBadInput, "blocksize cannot be zero") | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIV-01-001: Possible DoS via index out of range (Low) During the fuzzing process of the minivpn/vpn package, it was found that the parseServerHardResetPacket function fails to access the first element of the serverHardReset payload when the payload is empty. This led to the following crash error: “panic: runtime error: index out of range [0] with length 0”. Please note that the parseServerHardResetPacket function is used in association with the newServerHardReset method which checks that the payload is not empty and returns an error otherwise. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-001
During the fuzzing process of the minivpn/vpn package, it was found that the bytesPadPKCS7 function fails to perform a modulo operation when the blockSize argument is zero. This led to the following crash error: “panic: runtime error: integer divide by zero”. This issue affects projects using minivpn/vpn as a third party library or copying the vulnerable code into another project, if an attacker is able to control the blockSize value. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-002
afcebea
to
3022603
Compare
MIV-01-003: Possible DoS via nil Pointer Dereference (Medium) During the fuzzing process of the minivpn/vpn package, it was found that the EncryptAndEncodePayload function fails when dcs.dataCipher is null. This issue led to a nil pointer dereference with the following crash error: “panic: runtime error: invalid memory address or nil pointer dereference”. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-003
MIV-01-004: Possible DoS via Index Out of Range (Medium) During the fuzzing process of the minivpn/vpn package, it was found that the maybeAddCompressPadding function fails to validate access to the array of byte location provided as an argument. This led to the following crash error: “panic: runtime error: index out of range [-1]”. Please note the EncryptAndEncodePayload function, which invokes maybeAddCompressPadding, fails to perform the length check as well. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-004
MIV-01-005: Possible DoS via Slice Bounds Out of Range (High) During the fuzzing process of the minivpn/vpn package, it was found that the decodeEncryptedPayloadNonAEAD function fails to perform slicing when buf is not long enough. This led to the following crash error: “panic: runtime error: slice bounds out of range [:36] with capacity 32”. Thanks to 7asecurity that uncovered this bug during their Red Lab engagement. - Reference: MIV-01-005
Here I add a simple retry strategy that increments the port by one if the default or configured listening port is in use. While used as a library, it is be the caller's responsibility to catch the error and retry accordingly, this retry strategy is only provided for convenience while using the reference executable. - Reference: MIV-01-007
MIV-01-008 Possible File Disclosure via Error Messages (Info) It was found that the minivpn client reveals the contents of local files via error messages based on the user-supplied configuration path. A malicious attacker might leverage this weakness to fool some minivpn users, in order to gain access to data in local system files from the victim computer. This might be accomplished through social engineering, for example, providing fake minivpn instructions to a victim user and asking for the resulting minivpn errors via email or instant messaging. Fix consists of referencing the line number instead of the line content. - Reference: MIV-01-008
Otherwise, bogus provider names will lead to the creation of arbitrary folders. - Reference: MIV-01-011
On the topic of the canary stack protections, check golang/go#21871 (comment) - Reference: MIV-01-010
Revert a change by which we had ceased to explicitely set min and max TLS version. Apparently uTLS does not pick those from the passed clienthelo spec. Thanks to 7asecurity for raising this issue during their security audit. - Reference: MIV-01-012
As pointed out by the security audit, the use of P_DATA_V1 format was too conspicuous. In order to be able to use the v2 format, we needed to implement a few requirements: - client needs to push a specific IV_PROTO bit, so that the server knows that they have to assign us a peer-id. - client needs to parse the pushed peer-id from the options pushed by the server. - this peer-id is needed to encrypt (is passed to the authenticated data in GCM/AEAD mode) and to decrypt data payloads. As a note, the peer-id expires in the server some time after the client has terminated the session. Useful pointers: https://build.openvpn.net/doxygen/group__data__crypto.html https://sourceforge.net/p/openvpn/mailman/message/32979583/ https://community.openvpn.net/openvpn/attachment/ticket/268/0001-Always-push-basic-set-of-peer-info-values-to-server-v2.patch
3022603
to
197ce1a
Compare
decompression was only working for AEAD (GCM) before. the compress=stub case seems not to be working when compressing, but since the compress option appears to be deprecated in openvpn 2.5.x we will not care about this case for the time being.
this was a pending refactor: we really don't need to instatiate the hmac each time. we also don't need the getHashLength function anymore.
@@ -113,6 +113,9 @@ func bytesUnpadPKCS7(b []byte, blockSize int) ([]byte, error) { | |||
|
|||
// bytesPadPKCS7 returns the PKCS#7 padding of a byte array. | |||
func bytesPadPKCS7(b []byte, blockSize int) ([]byte, error) { | |||
if blockSize == 0 { | |||
return nil, fmt.Errorf("%w: %s", errBadInput, "blocksize cannot be zero") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-002 Possible DoS via Integer Division by Zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are at it, I think we should also protect against negative blockSize values. The assumption in the report is that the attacker may be able to control the blockSize
value, hence if that is the case, this snippet will also lead to a crash:
b := []byte("a")
blockSize := -1
psiz := blockSize - len(b) % blockSize
padding := bytes.Repeat([]byte{byte(psiz)}, psiz)
via
panic: bytes: negative Repeat count
goroutine 1 [running]:
bytes.Repeat(0xc0000a2f27, 0x1, 0x1, 0xffffffffffffffff, 0x46, 0x4db500, 0xc000010080)
/usr/local/go/src/bytes/bytes.go:499 +0x1aa
main.main()
/home/runner/3bh1srnac6d/main.go:11 +0x62
return []byte{}, fmt.Errorf("%w: nothing to encrypt", errCannotEncrypt) | ||
} | ||
if dcs == nil || dcs.dataCipher == nil { | ||
return []byte{}, fmt.Errorf("%w: %s", errCannotEncrypt, fmt.Errorf("data chan not initialized")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-003 Possible DoS via nil Pointer Dereference
👍 for also adding regress tests for it
func maybeAddCompressPadding(b []byte, compress compression, blockSize uint8) ([]byte, error) { | ||
func doPadding(b []byte, compress compression, blockSize uint8) ([]byte, error) { | ||
if len(b) == 0 { | ||
return nil, fmt.Errorf("%w: nothing to pad", errBadInput) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-004 Possible DoS via Index Out of Range
@@ -502,37 +540,36 @@ func decodeEncryptedPayloadAEAD(buf []byte, state *dataChannelState) (*encrypted | |||
encrypted := &encryptedData{ | |||
iv: iv.Bytes(), | |||
ciphertext: payload.Bytes(), | |||
aead: packet_id, | |||
aead: headers.Bytes(), | |||
} | |||
return encrypted, nil | |||
} | |||
|
|||
func decodeEncryptedPayloadNonAEAD(buf []byte, state *dataChannelState) (*encryptedData, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-005 Possible DoS via Slice Bounds Out of Range
It is my understanding that a fix for this has not been implemented. Yet according to the audit report it seems like this is a flaw in the protocol design of OpenVPN and the issue is present in the reference implementation as well. Apparently the issue is not so severe in there, because a retry mechanism is in place. While the retry mechanism might be effective as a protection against bad network conditions, I don't think it's really effective as a protection against a motivated attacker (such as a state actor or an ISP). In that case since they control the traffic flows, they can just pre all traffic that looks like OpenVPN (or that doesn't look like something they would recognise) with a bad payload leading to the connection never establishing. In light of the fact that our primary goal for this client is measurement, we actually care to record these kinds of connectivity issues and while we might want to add retries eventually to understand the persistence of the attacker and rule out potential transient network conditions, I don't think it's necessarily critical to solve this in order to land this. |
if err := server.ListenAndServe("tcp", addr); err != nil { | ||
panic(err) | ||
if isErrorAddressAlreadyInUse(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you could perhaps just call server.ListendAndServe
with addr = {ip}:0
so as to make the OS pick the next available port for you.
I then saw, though, that the sock5 library you are using doesn't return the net.listener
(https://github.com/armon/go-socks5/blob/master/socks5.go#L100) so if you actually need to know the picked port you have no way of extracting it :(
I guess we can leave this as-is for the time being, but it might be worth opening an issue as future work to open PR on https://github.com/armon/go-socks5 to return the listener (or set it as an attribute to the server struct).
- MIV-01-007 Possible DoS via Predictable Port Usage
@@ -335,13 +357,13 @@ func newServerHardReset(b []byte) (*serverHardReset, error) { | |||
// parseServerHardResetPacket returns the sessionID received from the server, or an | |||
// error if we could not parse the message. | |||
func parseServerHardResetPacket(p *serverHardReset) (sessionID, error) { | |||
if len(p.payload) < 10 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-001 Possible DoS via index out of range
@@ -437,7 +459,7 @@ func parseOption(o *Options, dir, key string, p []string) error { | |||
return e | |||
} | |||
default: | |||
log.Println("warn: unsupported key:", key) | |||
log.Printf("warn: unsupported key in line %d\n", lineno) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-008 Possible File Disclosure via Error Messages
This seems very hard to do. I think we ought to document this as future work. |
os.exit(1) | ||
|
||
sys.exit(1) | ||
if sys.argv[1] not in PROVIDERS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-011 Missing Verification on VPN bootstrap-provider Utility Script
@@ -199,6 +197,9 @@ func initTLS(session *session, cfg *certConfig) (*tls.Config, error) { | |||
VerifyPeerCertificate: customVerify, | |||
// disable DynamicRecordSizing to lower distinguishability. | |||
DynamicRecordSizingDisabled: true, | |||
// uTLS does not pick min/max version from the passed spec | |||
MinVersion: tls.VersionTLS12, | |||
MaxVersion: tls.VersionTLS13, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- MIV-01-012 Possible MitM due to Missing TLS MinVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed the PR comparing it against the report.
I think the only thing that should be addressed prior to merge is:
- adding checks for negative blockSize values (see: https://github.com/ooni/minivpn/pull/23/files#r1038019424)
Beyond that I think we ought to document as future work the harder bits that are left un-addressed, but I don't think that should be blocking on landing this.
Thanks for working on this!
thanks for the review @hellais re. I am going to go ahead and fix the issue with blockSize and merge this series, so that we can rebase the remaining PRs. |
yes, I think this is a wontfix for the moment. however, since I wrote this patch series I've revisited the reliability layer implementation and I think it's quite close to be in an usable state (I keep debugging it a bit, and is missing proper tests). In my priority lists for further work in minivpn, I think implementing proper DoS protection ( on the other hand, we want to be able to measure DoS happening - I think the trick is to be as sensitive as openvpn itself. |
in progress: address security fixes found by 7asecurity.